Add metrics for connection pool usage v3 #307
Conversation
|
The pool ID should be the name of the Ecto repo, no? My biggest concern is that those costs will be paid by everyone in every operation, regardless of the sampling or need. So I think we should try polling first and fallback to telemetry only if that doesn’t work. Or put the telemetry in this PR behind some periodic action. |
|
@josevalim |
|
Thank you, this looks great! I think we just need to move it to the |
|
Fair enough but this will introduce a breaking change into the API. |
|
Actually, it isn't a part of the public API but I guess it should be a part of both DBConnection.Pool and DBConnection. |
realglebivanov
changed the title
DBConnection.ConnectionPool
|
Yes, the pool API is private, so we are fine (but it would be fine to add new callbacks to behaviours nonetheless). |
…ection_metrics a part of DBConnection public API
|
@josevalim I've managed to come up with a sketch of what the public API might look like. Please, take a look! |
|
Thanks, José! |
| {:ok, pool_metrics} = DBConnection.ConnectionPool.get_connection_metrics(pool) | ||
|
|
||
| proxy_metrics = | ||
| owners | ||
| |> Enum.map(fn {_, {proxy, _, _}} -> | ||
| if Process.alive?(proxy) do | ||
| GenServer.call(proxy, :get_connection_metrics) | ||
| end | ||
| end) | ||
| |> Enum.reject(&is_nil/1) | ||
|
|
||
| {:reply, {:ok, pool_metrics ++ proxy_metrics}, state} | ||
| catch | ||
| :exit, reason -> | ||
| if log do | ||
| Logger.log( | ||
| log, | ||
| "Caught :exit while calling :get_connection_metrics due to #{inspect(reason)}" | ||
| ) | ||
| end | ||
|
|
||
| {:reply, :error, state} |
There was a problem hiding this comment.
| {:ok, pool_metrics} = DBConnection.ConnectionPool.get_connection_metrics(pool) | |
| proxy_metrics = | |
| owners | |
| |> Enum.map(fn {_, {proxy, _, _}} -> | |
| if Process.alive?(proxy) do | |
| GenServer.call(proxy, :get_connection_metrics) | |
| end | |
| end) | |
| |> Enum.reject(&is_nil/1) | |
| {:reply, {:ok, pool_metrics ++ proxy_metrics}, state} | |
| catch | |
| :exit, reason -> | |
| if log do | |
| Logger.log( | |
| log, | |
| "Caught :exit while calling :get_connection_metrics due to #{inspect(reason)}" | |
| ) | |
| end | |
| {:reply, :error, state} | |
| {:ok, pool_metrics} = DBConnection.ConnectionPool.get_connection_metrics(pool) | |
| proxy_metrics = | |
| owners | |
| |> Enum.map(fn {_, {proxy, _, _}} -> | |
| try do | |
| GenServer.call(proxy, :get_connection_metrics, :infinity) | |
| catch | |
| :exit, _ when log -> | |
| if log do | |
| Logger.log( | |
| log, | |
| "Caught :exit while calling :get_connection_metrics due to #{inspect(reason)}" | |
| ) | |
| end | |
| nil | |
| end | |
| end) | |
| |> Enum.reject(&is_nil/1) | |
| {:reply, {:ok, pool_metrics ++ proxy_metrics}, state} |
There was a problem hiding this comment.
Here is the suggestion. The poll is linked to the current process, so we don't have to worry about it crashing, cause it would crash us too. There is a chance of timeout, but we can also just pass :infinity as the timeout (which I recommend anyway).
The only pending question is: do you want to return {:ok, metrics} or just the metrics directly?
There was a problem hiding this comment.
We can't return just metrics because it's the part of DBConnection.Pool behavior.
By the way, do we really want the client of get_connection_metrics to wait indefinitely for a response or handle timeouts on its own?
There was a problem hiding this comment.
Actually, we've gotten rid of all the potential errors. We can probably get rid of maybeish type in the public API as well
There was a problem hiding this comment.
By the way, do we really want the client of get_connection_metrics to wait indefinitely for a response
Why would we wait indefinitely? A timeout is a safe default if we have blocking code but that's not the case here. The whole pool is design to queue things immediately instead of blocking.
There was a problem hiding this comment.
I meant the GenServer.call calls with timeout = :infinity which in theory could cause indefinite wait times
There was a problem hiding this comment.
I get what you mean but I didn't know all the ins and outs of :gen and that's why the first decision was not to risk with :infinity.
Anyway, I've updated the code.
Co-authored-by: José Valim <[email protected]>
Co-authored-by: José Valim <[email protected]>
|
💚 💙 💜 💛 ❤️ |
|
❤️ i'm a contributor now 🤣 |
|
And not an easy feature to add, well done! |
Hello!
Since these two PRs seem to be abandoned
#306
#292,
I've decided to open another one that will hopefully be merged.